Provide Custom Info as Pillar data #3093

nadvornik · 2021-01-07T14:29:54Z

What does this PR change?

Provide Custom Info as Pillar data

GUI diff

No difference.

Documentation

TBD
DONE

Test coverage

Unit tests were added
DONE

Links

Fixes https://github.com/SUSE/spacewalk/issues/12016

DONE

Changelogs

If you don't need a changelog check, please mark this checkbox:

No changelog needed

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Re-run a test

If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:

Re-run test "changelog_test"
Re-run test "backend_unittests_pgsql"
Re-run test "java_pgsql_tests"
Re-run test "schema_migration_test_oracle"
Re-run test "schema_migration_test_pgsql"
Re-run test "susemanager_unittests"
Re-run test "javascript_lint"
Re-run test "spacecmd_unittests"

moio

Thanks for the efforts here, I am being a bit nitpicky but I think we are already on a good path!

Just not to forget, once the code is done please also open an issue or PR to our docs, this is a small but nice piece of news to share for the scripters out there.

Thanks, it's been a long time since I hoped this will one day be done!

moio · 2021-01-11T13:43:21Z

java/code/src/com/redhat/rhn/frontend/xmlrpc/system/SystemHandler.java

@@ -2075,6 +2076,10 @@ public int setCustomValues(User loggedInUser, Integer sid, Map<String, String> v
            }
        }

+        server.asMinionServer().ifPresent(minion -> {
+            MinionPillarManager.INSTANCE.generatePillar(minion);


I know it's a bit more work, but I'd encourage creating an ad-hoc MinionPillarGenerator for this use case, or any change to custom info will entail the full regeneration of all pillar data, which can take a bit of time.

Do you mean adding new pillar file or calling only MinionGeneralPillarGenerator?
Anyway, if I understand correctly, the write should go via MinionPillarManager.INSTANCE, right?
Is it OK to change MinionPillarManager to use named variables instead of list to hold MinionPillarFileManager instances and add MinionPillarManager.generatePillar with arguments to use only selected MinionPillarFileManager ?

Do you mean adding new pillar file or calling only MinionGeneralPillarGenerator?

I meant adding a new pillar file and related Generator class. Actually I made a copypaste mistake in that comment - meaning MinionPillarGenerator but actually writing MinionGeneralPillarGenerator, sorry for having confused you.

Anyway, if I understand correctly, the write should go via MinionPillarManager.INSTANCE, right?

Only if you want to regenerate all pillar data, which IIUC should not be the case here.

Is it OK to change MinionPillarManager to use named variables instead of list to hold MinionPillarFileManager instances and add MinionPillarManager.generatePillar with arguments to use only selected MinionPillarFileManager ?

It's not a bad idea FMPOV, the only thing I see is that, so far, the pattern has been to just instantiate a new MinionPillarFileManager(new MySpecificPillarGenerator()) and call updatePillarFile().

So I would welcome such change, at the condition it comes in its own separate commit (or even PR if you so prefer) and that all similar cases are changed in one shot (I see three of them apart from tests, see callers of MinionPillarGenerator.generatePillarData() as displayed by IntelliJ's Navigate -> Call Hierarchy).

OK, I am working on it.
For completeness, ad-hoc MinionPillarFileManager instances do not work with unit tests which change pillar root.

One more reason to go ahead with such a refactoring!

moio · 2021-01-11T13:54:49Z

java/code/src/com/redhat/rhn/frontend/action/systems/sdc/UpdateCustomDataAction.java

@@ -134,6 +135,9 @@ public ActionForward execute(ActionMapping mapping,
            cdv.setCreator(user);
            cdv.setLastModifier(user);
            request.setAttribute(VAL_PARAM, form.get(VAL_PARAM));
+            server.asMinionServer().ifPresent(minion -> {
+                MinionPillarManager.INSTANCE.generatePillar(minion);
+            });


I am a bit confused by the logic from lines above this. It seems to me that addCustomDataValue already creates a new CustomDataValue instance and sets it if null, and now we have the logic repeated here.

I would not mind in the context of this PR normally, but here we are adding code which is clearly back-end (pillar generation) to a front-end class (an Action), and was wondering where it would be placed best.

Would it be possible to fold the code above and the pillar generation inside of addCustomDataValue - either to be left in Server or moved into ServerFactory?

lucidd · 2021-01-29T14:36:32Z

java/code/src/com/suse/manager/webui/services/pillar/MinionPillarManager.java

+     * @param minion the minion server
+     * @param subsets subsets of pillar, that should be generated
+     */
+    public void generatePillar(MinionServer minion, EnumSet<PillarSubset> subsets) {


I think the EnumSet is not really needed and makes it less convenient to use. Usually EnumSet is a good way to represent flags in a typesafe way while still retaining the performance of bitfields but in this case we are just giving a list of things we want to iterate over and do something for each of them. Instead if would just do PillarSubset... (varargs) so you can just pass multiple PillarSubset as arguments without any additional wrapping. The method would then be just iterating over it with a switch.

lucidd · 2021-01-29T14:39:24Z

java/code/src/com/suse/manager/webui/services/pillar/MinionPillarManager.java

+     * @param subsets subsets of pillar, that should be generated
+     */
+    public void generatePillar(MinionServer minion, EnumSet<PillarSubset> subsets) {
+        if (subsets.contains(PillarSubset.ACCESS_TOKENS)) {


ACCESS_TOKENS does not seem to fit in with the rest as it does not generate pillar data but updates data for GENERAL. I think it would be more consistent adding boolean refreshAccessTokens as an extra parameter like its the case in the other method already.

lucidd · 2021-01-29T14:55:38Z

java/code/src/com/redhat/rhn/domain/server/Server.java

+
+        if (updatePillar) {
+            this.asMinionServer().ifPresent(minion -> {
+                MinionPillarManager.INSTANCE.generatePillar(minion,


In my recent refactoring i started to reduce the usage of global static instances and tried to give a clearer picture of dependencies of different aspects of the codebase. Having the server (data class) directly generate its pillar data breaks that in a big way. I would suggest moving this code at least to the call site.

nadvornik · 2021-03-16T15:44:03Z

Updated as requested.

Ad-hoc MinionPillarFileManager instances do not work with unit tests, which change pillar root globally. With this change all pilar writes can go via MinionPillarManager.INSTANCE but it is still possible to generate only particular pillar file, if needed. Adjusted unit tests.

nadvornik changed the title ~~Custominfo~~ Provide Custom Info as Pillar data Jan 7, 2021

nadvornik requested a review from moio January 7, 2021 14:33

moio reviewed Jan 11, 2021

View reviewed changes

nadvornik force-pushed the custominfo branch 4 times, most recently from 0e88c8e to 5557d9e Compare January 15, 2021 13:07

nadvornik requested a review from moio January 15, 2021 14:05

github-actions bot added java java_lint_checkstyle java_pgsql_tests labels Jan 15, 2021

lucidd self-requested a review January 29, 2021 14:49

lucidd reviewed Jan 29, 2021

View reviewed changes

github-actions bot added data-model xmlrpc labels Mar 11, 2021

nadvornik force-pushed the custominfo branch 3 times, most recently from c5c9005 to c92f280 Compare March 16, 2021 14:56

nadvornik requested a review from lucidd March 16, 2021 15:44

lucidd approved these changes Mar 29, 2021

View reviewed changes

nadvornik added 6 commits March 30, 2021 17:10

Provide Custom Info as Pillar

0646322

Test for Custom Info in Pillar

58e8bc8

Fix UpdateCustomDataAction

3fb0a03

Put Custom Info in separate pillar file

3ec086c

Update changelogs

7e4bb9d

nadvornik force-pushed the custominfo branch from c92f280 to 7e4bb9d Compare March 30, 2021 15:15

nadvornik merged commit c3c577b into uyuni-project:master Mar 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide Custom Info as Pillar data #3093

Provide Custom Info as Pillar data #3093

nadvornik commented Jan 7, 2021

moio left a comment

moio Jan 11, 2021

nadvornik Jan 12, 2021

moio Jan 12, 2021

nadvornik Jan 13, 2021

moio Jan 14, 2021

moio Jan 11, 2021

lucidd Jan 29, 2021

lucidd Jan 29, 2021

lucidd Jan 29, 2021

nadvornik commented Mar 16, 2021

Provide Custom Info as Pillar data #3093

Provide Custom Info as Pillar data #3093

Conversation

nadvornik commented Jan 7, 2021

What does this PR change?

GUI diff

Documentation

Test coverage

Links

Changelogs

Re-run a test

moio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nadvornik commented Mar 16, 2021